Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move WindowOrWorkerGlobalScope features to api.* (_globals folder) #11518

Merged
merged 4 commits into from
Aug 31, 2021

Conversation

Elchi3
Copy link
Member

@Elchi3 Elchi3 commented Jul 14, 2021

This PR illustrates unifying Window and WorkerGlobalScope in a globals.json file and marking worker support in sub features.

The alternate approach is a split, see #11517

Context: #9127

@Elchi3 Elchi3 requested a review from ddbeck as a code owner July 14, 2021 10:29
@Elchi3 Elchi3 marked this pull request as draft July 14, 2021 10:29
@github-actions github-actions bot added data:api 🐇 Compat data for Web APIs. https://developer.mozilla.org/docs/Web/API schema ⚙️ Isses or pull requests regarding the JSON schema files used in this project. labels Jul 14, 2021
@ddbeck
Copy link
Collaborator

ddbeck commented Jul 14, 2021

I'm still undecided on splitting or lumping here, but I do have thoughts on this:

the schema doesn't like this, it wants these exports in one file per export

Seems to me, since we're doing something quite similar to mixins in this approach, that we could do something similar and avoid changing the schema:

 _globals/
  ├── atob.json
  ├── btoa.json
  ├── caches.json
  └── clearInterval.json

@Elchi3
Copy link
Member Author

Elchi3 commented Jul 14, 2021

That seems a lot better, yes. (although more work).

What do we define as "globals", though? I guess there are more features that qualify outside of WindowOrWorkerGlobalScope?

@foolip
Copy link
Collaborator

foolip commented Jul 14, 2021

There are a few additional properties that are on the global object in both window and worker contexts to consider here, namely location, navigator and performance.

location and navigator don't have the same type in both contexts and thus don't have identical IDL definitions for both, so leaving those separate is probably a good idea. But performance I think should be a global api.performance.

There's also the interesting case of close() which exists in both contexts but is not the same:
https://developer.mozilla.org/en-US/docs/Web/API/Window/close
https://developer.mozilla.org/en-US/docs/Web/API/WorkerGlobalScope/close

Similarly, both have a self property, and while it could be described in the same terms, they are usefully documented separately:
https://developer.mozilla.org/en-US/docs/Web/API/Window/self
https://developer.mozilla.org/en-US/docs/Web/API/WorkerGlobalScope/self

@foolip
Copy link
Collaborator

foolip commented Jul 14, 2021

Oh, performance is actually defined using the WindowOrWorkerGlobalScope mixin:
https://w3c.github.io/hr-time/#the-performance-attribute

@foolip
Copy link
Collaborator

foolip commented Jul 14, 2021

What do we define as "globals", though? I guess there are more features that qualify outside of WindowOrWorkerGlobalScope?

Indeed, there are some APIs available only on Window which nevertheless similar to fetch() or performance in how they're used without Window being especially relevant. Candidates:

  • alert() and friends
  • crypto
  • history
  • localStorage and sessionStorage
  • requestIdleCallback
  • speechSynthesis

Some are actually pretty entwined with the main thread, history most obviously so, but it's not super clear how to draw the line here.

I would be happy enough with a guidelines saying that only things available in more than one global scope should be given the globals treatment. An objective rule would at least make it easier on tooling (like mdn-bcd-collector) to map from Web IDL to BCD paths.

@Elchi3
Copy link
Member Author

Elchi3 commented Jul 15, 2021

Seems to me, since we're doing something quite similar to mixins in this approach, that we could do something similar and avoid changing the schema:

 _globals/
  ├── atob.json
  ├── btoa.json
  ├── caches.json
  └── clearInterval.json

Done! Much better

@Elchi3
Copy link
Member Author

Elchi3 commented Jul 15, 2021

I would be happy enough with a guidelines saying that only things available in more than one global scope should be given the globals treatment. An objective rule would at least make it easier on tooling (like mdn-bcd-collector) to map from Web IDL to BCD paths.

Right, it would be good to get a list of "globals" derived from Web IDL syntax.

@teoli2003
Copy link
Contributor

Question: how do we deal with recent global scopes like AudioWorkletGlobalScope ? console is available on it (as it is a namespace), but most other "global" properties/methods like atob not.

@foolip
Copy link
Collaborator

foolip commented Jul 15, 2021

Question: how do we deal with recent global scopes like AudioWorkletGlobalScope ? console is available on it (as it is a namespace), but most other "global" properties/methods like atob not.

It's not really because it's a namespace, rather it's because of [Exposed=(Window,Worker,Worklet)] in https://console.spec.whatwg.org/#console-namespace which can also be used for interfaces, for example here:
https://streams.spec.whatwg.org/#rs-class-definition

@foolip
Copy link
Collaborator

foolip commented Jul 15, 2021

it would be good to get a list of "globals" derived from Web IDL syntax.

The only way of defining global method or properties using Web IDL is to add stuff to Window, WorkerGlobalScope or another interface representing a global scope. (These use the Global extended attribute and are thus easy to enumerate.)

I would say only methods or properties added to more than one global scope should count. The tricky bit is determining from Web IDL if they are the same, or just share a name. For WindowOrWorkerGlobalScope it can be inferred, but there's no WindowOrWorkerOrWorkletGlobalScope yet. How exactly specs use mixins vs. partials might change over time.

So, even though a rule involving WindowOrWorkerGlobalScope might work now, I think we should rather say that it's a global if it's exposed in two or more globals and it has the same definition (likely spec_url) in both cases. That avoids the two close() methods with exactly the same signature being treated as a global:
https://html.spec.whatwg.org/multipage/window-object.html#dom-window-close
https://html.spec.whatwg.org/multipage/workers.html#dom-dedicatedworkerglobalscope-close

This does require human judgement, but I think in practice it will be possible to infer from the state of Web IDL at any given time, it's just that the rule might have to evolve with changes to specs and Web IDL itself.

Another option I think we should discard:

Simplest possible rule

The simplest we could do is to say that anything exposed on any global object is a global. Unfortunately, this leads to the silly conclusion that window.resizeTo() is a global, when the window is the thing being resized. It's very unlike fetch(), which can be understood without reference to window.

This also wouldn't work for close(), which has the same name but does something different for Window and WorkerGlobalScope. And it makes no sense for the properties of AudioWorkletGlobalScope.

@foolip
Copy link
Collaborator

foolip commented Jul 15, 2021

@Elchi3 do you want to change the title of this PR? It's not unifying all of Window and WorkerGlobalScope, but extracting and deduplicating some bits.

@Elchi3 Elchi3 changed the title Unify Window and WorkerGlobalScope Move WindowOrWorkerGlobalScope features to api.* (_globals folder) Jul 16, 2021
@ddbeck ddbeck removed their request for review August 3, 2021 10:45
@Elchi3 Elchi3 marked this pull request as ready for review August 20, 2021 09:19
@Elchi3
Copy link
Member Author

Elchi3 commented Aug 20, 2021

I've closed #11517. We'd like to go ahead with this PR.

@foolip
Copy link
Collaborator

foolip commented Aug 20, 2021

@Elchi3 can you resolve conflicts? Even without conflicts there's probably a risk that something has changed since the PR was created, so how to verify that this isn't accidentally reverting any data?

@Elchi3
Copy link
Member Author

Elchi3 commented Aug 20, 2021

Ah yes, I should probably re-do this entirely.

@Elchi3 Elchi3 removed the schema ⚙️ Isses or pull requests regarding the JSON schema files used in this project. label Aug 25, 2021
@Elchi3 Elchi3 added the needs content update 📝 This PR needs a corresponding update to mdn/content to update the documentation label Aug 25, 2021
@Elchi3
Copy link
Member Author

Elchi3 commented Aug 26, 2021

Okay running into difficulties here...

I can't move api.window.crypto to api.crypto because it collides with api.Crypto.
BCD might support this (exports are case-sensitive), but on MDN you have lowercase folders only, so I can't have this folder twice: content/files/en-us/web/api/crypto.

This reverts commit 998bc0a.
@Elchi3
Copy link
Member Author

Elchi3 commented Aug 30, 2021

I reverted the crypto change. I think we should deal with it in a follow-up.

@sideshowbarker sideshowbarker merged commit fb2ac55 into mdn:main Aug 31, 2021
Elchi3 added a commit to Elchi3/browser-compat-data that referenced this pull request Sep 1, 2021
Elchi3 added a commit to Elchi3/browser-compat-data that referenced this pull request Sep 1, 2021
@Elchi3 Elchi3 deleted the window-worker-unified branch September 2, 2021 12:33
Elchi3 added a commit that referenced this pull request Sep 2, 2021
* Bump version to v4.0.2

* Add release note for #12187

* Add release note for #12186

* Hoist known issues

* Add release note for #11518

* Add release stats and date

* Fix md lint and link

* Feedback

Co-authored-by: Florian Scholz <fs@florianscholz.com>
@saschanaz
Copy link
Contributor

This sounds like a major change at least for code generators, not sure why it didn't even got a minor version up 👀

@Elchi3
Copy link
Member Author

Elchi3 commented Sep 3, 2021

Thanks for your feedback. I released this according to https://github.com/mdn/browser-compat-data#semantic-versioning-policy.

You should expect lower-level namespaces, feature data, and browser data to be added, removed, or modified at any time. That said, we strive to communicate changes and preserve backward compatibility; if you rely on a currently undocumented portion of the package and want SemVer to apply to it, please open an issue.

Like other mixins, WindowOrWorkerGlobalScope was not a protected namespace per this policy. However, I'd like to understand how you consume BCD and what needs to be taken into account for "code generators".

@foolip
Copy link
Collaborator

foolip commented Sep 3, 2021

Some adaptation in mdn-bcd-collector will be needed too. The problem is basically how to create a mapping between Web IDL and BCD paths. Previously, assuming mixin flattening, the path for fetch() would have been api.Window.fetch + api.WorkerGlobalScope.fetch. Now it should instead be api.fetch and api.fetch.worker_support.

@Elchi3
Copy link
Member Author

Elchi3 commented Sep 3, 2021

So mixin flattening would have been the preferred approach? I'm a bit puzzled now...

@saschanaz
Copy link
Contributor

Non-breaking rather than preferred, I guess

@foolip
Copy link
Collaborator

foolip commented Sep 3, 2021

No, this is exactly the approach I prefer, I'm just saying what changes will be needed in IDL<->BCD mapping.

@queengooborg
Copy link
Collaborator

I personally think that this was a significant enough change that a minor bump should have been made, if not a major bump. Consumers have shown concern for when features have moved around or been removed, and I feel that we should help communicate these changes with our version numbers. A change like this was versioned properly according to our guidelines, but maybe we should revise them a little?

@ddbeck ddbeck removed needs-release-note 📰 needs content update 📝 This PR needs a corresponding update to mdn/content to update the documentation labels Sep 10, 2021
foolip added a commit to foolip/browser-compat-data that referenced this pull request Sep 17, 2021
This was missed in mdn#11518.

This is defined using WindowOrWorkerGlobalScope in the spec:
https://w3c.github.io/hr-time/#the-performance-attribute

None of the data was validated or changed, just moved.
Elchi3 added a commit that referenced this pull request Sep 29, 2021
* Move the performance attribute to api.performance (in _globals)

This was missed in #11518.

This is defined using WindowOrWorkerGlobalScope in the spec:
https://w3c.github.io/hr-time/#the-performance-attribute

None of the data was validated or changed, just moved.

* Update api/_globals/performance.json

Co-authored-by: Florian Scholz <fs@florianscholz.com>

Co-authored-by: Florian Scholz <fs@florianscholz.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
data:api 🐇 Compat data for Web APIs. https://developer.mozilla.org/docs/Web/API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants